improvement: Support completions for implicit classes #5904

tgodzik · 2023-12-01T13:11:25Z

Previously, we would only automatically suggest extension methods, not implicit classes.

Now, we also properly suggest implicit classes.

We could follow up with support for Scala 2 also.

Connected to #5893 <- I want to close it after also fixing Scala 2

kasiaMarek · 2023-12-04T16:36:27Z

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala

+           * artificial package object. Scala 2 doesn't allow for it.
+           */
+          val isScala3Implicit =
+            dialect.allowExtensionMethods && currRegion.produceSourceToplevel &&


I think you need to set the termOwner for current region or something to make sure we don't produce this artificial package object more than once. Though I'm not sure if that would cause any actual issues.

I added a class to input to test it. Should be correct now.

kasiaMarek · 2023-12-04T16:44:19Z

mtags/src/main/scala/scala/meta/internal/mtags/ScalaToplevelMtags.scala

+              term(name, pos, Kind.OBJECT, 0)
+            }
+            owner
+          } else currRegion.owner


Suggested change

} else currRegion.owner

} else currRegion.termOwner

I think that's the one you want here.

Should be correct now, though I had to do a couple of changes, They're in the second to last commit.

kasiaMarek · 2023-12-04T17:23:56Z

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala

+          def isImplicitClass(owner: Symbol) =
+            val constructorParam =
+              owner.info.allMembers
+                .find(_.symbol.isAllOf(Flags.PrivateParamAccessor))


Suggested change

.find(_.symbol.isAllOf(Flags.PrivateParamAccessor))

.find(_.symbol.isAllOf(Flags.ParamAccessor))

It seems it doesn't have to be private and

implicit class A (val num: Int): def incr: Int = num + 1

is legal.

You're right, fixed and changed one of the tests.

kasiaMarek · 2023-12-04T17:25:34Z

mtags/src/main/scala-3/scala/meta/internal/pc/SemanticdbSymbols.scala

+                    owner.info.allMembers
+                      .find(_.name == searched)
+                      .map(_.symbol)
+                      .toList


Wouldn't owner.info.member(termName(value)).symbol achieve the same?

Also can you in the comment mark for which part it doesn't work? (I assume you mean implicit class A, since that's the type.)

I agree, but maybe with typeName instead of termName?

Thanks, changed and it works!

kasiaMarek · 2023-12-04T17:28:10Z

tests/mtest/src/main/scala/tests/PCSuite.scala

@@ -100,7 +100,7 @@ trait PCSuite {
      case NonFatal(e) =>
        println(s"warn: ${e.getMessage}")
    }
-    workspace.inputs(filename) = (code2, dialect)
+    workspace.inputs(file.toURI.toString()) = (code2, dialect)


Pure curiosity, why did that change?

That was actually showing errors whenever running tests, wasn't really problematic, but spamming the logs.

jkciesluk · 2023-12-05T10:39:33Z

mtags/src/main/scala-3/scala/meta/internal/pc/CompilerSearchVisitor.scala

@@ -21,7 +22,12 @@ class CompilerSearchVisitor(
  val logger: Logger = Logger.getLogger(classOf[CompilerSearchVisitor].getName)

  private def isAccessible(sym: Symbol): Boolean = try
-    sym != NoSymbol && sym.isPublic && sym.isStatic
+    sym != NoSymbol && sym.isPublic && sym.isStatic || {
+      val owner = sym.maybeOwner


can we extract this part to seperate method like isAccessibleImplicit ?

We can also check whether the symbol is accessible from site, by using the compiler's sym.isAccessibleFrom(qualType) at CompilerSearchVisitor creation site

Good idea @rochala though at this point it would be a much bigger refactor, so I will just move it to a method.

jkciesluk · 2023-12-05T10:42:20Z

mtags/src/main/scala-3/scala/meta/internal/pc/SemanticdbSymbols.scala

+                    owner.info.allMembers
+                      .find(_.name == searched)
+                      .map(_.symbol)
+                      .toList


I agree, but maybe with typeName instead of termName?

mtags/src/main/scala-3/scala/meta/internal/pc/completions/Completions.scala

tests/cross/src/test/scala/tests/pc/CompletionExtensionMethodSuite.scala

Previously, we would only automatically suggest extension methods, not implicit classes. Now, we also properly suggest implicit classes. We could follow up with support for Scala 2 also.

jkciesluk

LGTM.
The tests are failing, but everything looks good, I think you need to rerun save-expect

kasiaMarek · 2023-12-18T14:54:19Z

tests/cross/src/main/scala/tests/BasePCSuite.scala

@@ -225,6 +225,15 @@ abstract class BasePCSuite extends BaseSuite with PCSuite {
  }

  object IgnoreScalaVersion {
+
+    def or(


I added IgnoreScalaVersion#and at some point, which (counterintuitively) is the same. Maybe delete one of these.

I removed the one I added, thanks!

kasiaMarek · 2023-12-18T15:07:00Z

tests/unit/src/test/resources/mtags-scala3/example/ToplevelImplicit.scala

+package example
+
+// This wasn't possible in Scala 2
+implicit class Xtension/*example.Xtension().*//*example.Xtension#*/(number/*example.Xtension#number.*/: Int) {


I see it's been like this all along but why do we emit a method for implicit class?

I haven't noticed it before, looks like this was done a long while ago 651a33f

And the comment is // emit symbol for implicit conversion, I guess this might no longer be needed 🤔

Previously, we would only support extension methods and not the old style implicit classes. Now, those are supported also. Introduced in scalameta/metals#5904

Previously, we would only support extension methods and not the old style implicit classes. Now, those are supported also. Introduced in scalameta/metals#5904 [Cherry-picked 3c5e216]

tgodzik requested review from kasiaMarek, rochala and jkciesluk December 1, 2023 13:11

tgodzik force-pushed the implicit-classes branch from 69fb9a1 to b1058ae Compare December 1, 2023 22:11

kasiaMarek reviewed Dec 4, 2023

View reviewed changes

jkciesluk reviewed Dec 5, 2023

View reviewed changes

tgodzik added 2 commits December 15, 2023 18:55

improvement: Support completions for implicit classes

24b2d19

Previously, we would only automatically suggest extension methods, not implicit classes. Now, we also properly suggest implicit classes. We could follow up with support for Scala 2 also.

bugfix: Only generate package object once

4ed074b

tgodzik force-pushed the implicit-classes branch from b1058ae to e643cf5 Compare December 15, 2023 19:02

jkciesluk approved these changes Dec 18, 2023

View reviewed changes

tgodzik force-pushed the implicit-classes branch 3 times, most recently from af70623 to 15f8b3e Compare December 18, 2023 12:19

tgodzik requested a review from kasiaMarek December 18, 2023 14:16

kasiaMarek approved these changes Dec 18, 2023

View reviewed changes

refactor: Tweaks after reviews

6684c12

tgodzik force-pushed the implicit-classes branch from 15f8b3e to 6684c12 Compare December 19, 2023 13:35

tgodzik merged commit e39acff into scalameta:main Dec 19, 2023
25 checks passed

tgodzik deleted the implicit-classes branch December 19, 2023 17:32

tgodzik mentioned this pull request Dec 20, 2023

improvement: Support completions for implicit classes scala/scala3#19314

Merged

filipwiech mentioned this pull request Feb 28, 2024

bugfix: completions for implicit methods #6156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement: Support completions for implicit classes #5904

improvement: Support completions for implicit classes #5904

tgodzik commented Dec 1, 2023

kasiaMarek Dec 4, 2023

tgodzik Dec 15, 2023

kasiaMarek Dec 4, 2023

tgodzik Dec 15, 2023

kasiaMarek Dec 4, 2023

tgodzik Dec 15, 2023

kasiaMarek Dec 4, 2023

jkciesluk Dec 5, 2023

tgodzik Dec 15, 2023

kasiaMarek Dec 4, 2023

tgodzik Dec 15, 2023

jkciesluk Dec 5, 2023

rochala Dec 5, 2023

tgodzik Dec 15, 2023

jkciesluk Dec 5, 2023

jkciesluk left a comment

kasiaMarek Dec 18, 2023

tgodzik Dec 19, 2023

kasiaMarek Dec 18, 2023

tgodzik Dec 19, 2023

	.find(_.symbol.isAllOf(Flags.PrivateParamAccessor))
	.find(_.symbol.isAllOf(Flags.ParamAccessor))

improvement: Support completions for implicit classes #5904

improvement: Support completions for implicit classes #5904

Conversation

tgodzik commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkciesluk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment